Skip to content

Fix: float[] to float in legacy bandwidth#918

Open
FirePheonix wants to merge 3 commits into
pysal:mainfrom
FirePheonix:fix/adaptive-bandwidth
Open

Fix: float[] to float in legacy bandwidth#918
FirePheonix wants to merge 3 commits into
pysal:mainfrom
FirePheonix:fix/adaptive-bandwidth

Conversation

@FirePheonix

Copy link
Copy Markdown
Contributor

In my last merged Pull Request #905:

bandwidth = bw_per_row[rows] * 1.0000001
# ... (some code)
d.data = _lps_kernel(d.data, bandwidth, ...)

on discussion with @sjsrey , here, it was passing bandwidth as an entire array of floats (float[]) directly into the kernel functions all at once for the entire sparse matrix. This caused issues because some custom kernel functions (or internal ones) expect bandwidth to be a single scalar float, which might lead to broadcasting errors.

Now, the code has been refactored to iterate through the data observation-by-observation (row-by-row):

bw_per_row = bw_per_row * 1.0000001
# ... (some codee)
for i in range(d_csr.shape[0]):
    start = d_csr.indptr[i]
    end = d_csr.indptr[i + 1]
    if start < end:
        d_csr.data[start:end] = func(
            d_csr.data[start:end], float(bw_per_row[i])
        )

i'm aware that numpy vectorization is definitely going to be faster, yet this change it's a necessary trade-off for correctness and API compliance. By iterating row by row, the code can pass a single scalar float to the kernel function.

So it completely resolves the issue by ensuring the kernel functions always receive a single float bandwidth, even when we are using adaptive (variable) bandwidths across the datasets..

I wrote a quick script to test both versions with a dataset of 100,000 points and a relatively large neighbor count ($k=50$), simulating a moderately intensive spatial operation.
Here are the benchmark results:
Older Code (Vectorized, but buggy): 11.2725 seconds
New Code (Iterative, fixed): 11.5369 seconds

it's only a 0.26 seconds tradeoff, i believe it's feasible to ensure api compliance.

@codecov

codecov Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.9%. Comparing base (95829c9) to head (5563cba).
⚠️ Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
libpysal/graph/_kernel.py 85.0% 3 Missing ⚠️
libpysal/graph/tests/test_builders.py 90.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #918     +/-   ##
=======================================
+ Coverage   85.6%   85.9%   +0.3%     
=======================================
  Files        151     151             
  Lines      16310   16524    +214     
=======================================
+ Hits       13967   14192    +225     
+ Misses      2343    2332     -11     
Files with missing lines Coverage Δ
libpysal/graph/tests/test_builders.py 99.7% <90.0%> (-0.3%) ⬇️
libpysal/graph/_kernel.py 85.7% <85.0%> (+2.8%) ⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@martinfleis

Copy link
Copy Markdown
Member

We'll need an example that reproduces the issue and which can be used in tests. If this is fixing some behaviour, a test should ensure that this is not reverted back in future.

@sjsrey

sjsrey commented Jun 2, 2026

Copy link
Copy Markdown
Member

We could, instead, refactor the upstream kernel functions to take either a scalar or array and do the right thing. This way you could keep the original vectorized implementation.

@knaaptime

Copy link
Copy Markdown
Member

that also looks jittable?

@martinfleis

Copy link
Copy Markdown
Member

We could, instead, refactor the upstream kernel functions to take either a scalar or array and do the right thing. This way you could keep the original vectorized implementation.

+1 on that.

Which kernel causes trouble? I am still waiting for the reproducer :)

@FirePheonix

FirePheonix commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@martinfleis , here is the explanation of the issue and how we can address it:

i searched and thought about possible failure and the failure occurs when a user passes a custom user-defined callable as the kernel parameter that contains scalar control flow (such as bounds checking for co-located points): (look at the following code below):

def safe_custom_kernel(distances, bw):
    # Prevents division by zero for co-located points
    if bw < 0.001: 
        bw = 0.001
    return np.exp(-distances / bw)

a test for this, could be, for example:
gsoc-2026 - Antigravity IDE - _kernel py (Working Tree) (_kernel py) 03-06-2026 01_50_51

on testing with the vectorized apporach:

gsoc-2026 - Antigravity IDE - _kernel py (Working Tree) (_kernel py) 03-06-2026 01_56_13

with iterative approach:
image

@FirePheonix

Copy link
Copy Markdown
Contributor Author

We could, instead, refactor the upstream kernel functions to take either a scalar or array and do the right thing. This way you could keep the original vectorized implementation.

was thinking the exact same..
(and, maybe i'm wrong too with scenario i have for user having a custom user-defined callable..)

@martinfleis

Copy link
Copy Markdown
Member

If it is only the case of custom callable, I think we should document that it should expect array input and behave correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants